Better integration for terminal links (better context-menu and add a tooltip)#2934
Better integration for terminal links (better context-menu and add a tooltip)#2934
Conversation
Deploying waveterm with
|
| Latest commit: |
95078bf
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f0cff112.waveterm.pages.dev |
| Branch Preview URL: | https://sawka-url-hover.waveterm.pages.dev |
WalkthroughAdds hover-link tooltip UI to the terminal view. Introduces TermTooltip and TermLinkTooltip components (Floating UI–based) and integrates them into TerminalView wrapped by a new NullErrorBoundary. Extends TermWrap with hoveredLinkUri and onLinkHover to report hover/leave events. Updates term context menu to derive URL from hovered link rather than selected text. Minor logging change in ErrorBoundary to use console.error. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| }, | ||
| }; | ||
| refs.setPositionReference(virtualEl); | ||
| }, [isOpen, mousePos?.x, mousePos?.y]); |
There was a problem hiding this comment.
WARNING: Missing refs in dependency array
The useLayoutEffect calls refs.setPositionReference() but refs is not included in the dependency array. While refs from useFloating is typically stable, it's better to include it for correctness and to avoid potential stale closure issues.
| }, [isOpen, mousePos?.x, mousePos?.y]); | |
| }, [isOpen, mousePos?.x, mousePos?.y, refs]); |
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
Files Reviewed (5 files)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
frontend/app/view/term/term-tooltip.tsx (1)
90-95:clearMaxTimeoutis recreated on every render; consider hoisting into auseRef-backed stable reference.Since the function only closes over
maxTimeoutRef(a stable ref), it doesn't need to be recreated each render. A common approach is to define it as auseCallbackwith an empty dependency array or inline it directly inside the effect.♻️ Proposed refactor
- const clearMaxTimeout = () => { - if (maxTimeoutRef.current != null) { - window.clearTimeout(maxTimeoutRef.current); - maxTimeoutRef.current = null; - } - }; - React.useEffect(() => { if (termWrap == null) { return; } + + const clearMaxTimeout = () => { + if (maxTimeoutRef.current != null) { + window.clearTimeout(maxTimeoutRef.current); + maxTimeoutRef.current = null; + } + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/view/term/term-tooltip.tsx` around lines 90 - 95, The helper clearMaxTimeout is being recreated every render even though it only uses the stable ref maxTimeoutRef; make it stable by memoizing it (e.g. wrap clearMaxTimeout in useCallback with an empty dependency array) or move its logic inline into the effect that needs it, ensuring references remain to maxTimeoutRef.current and you still call window.clearTimeout and set maxTimeoutRef.current = null; update any callers to use the new stable clearMaxTimeout reference.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/app/element/errorboundary.tsx`:
- Around line 48-50: Replace the console.log in the ErrorBoundary's
componentDidCatch with console.error so caught exceptions are recorded as
errors; locate the componentDidCatch method in
frontend/app/element/errorboundary.tsx (the method receiving parameters error:
Error, info: React.ErrorInfo) and change the logging call to
console.error(`${this.props.debugName ?? "NullErrorBoundary"} error boundary
caught error`, error, info) so devtools and monitoring treat it as an error.
In `@frontend/app/view/term/term-model.ts`:
- Around line 796-799: The menu label building uses hoveredURL.hostname which is
empty for non-HTTP(S) schemes; update the label in the menu.push call (the block
that constructs "Open URL (" + hoveredURL.hostname + ")") to fall back to a
meaningful value such as hoveredURL.href (or hoveredURL.protocol +
hoveredURL.pathname) when hoveredURL.hostname is falsy so the label becomes
"Open URL (…)" with the actual URI for file://, ssh://, and custom schemes.
In `@frontend/app/view/term/term-tooltip.tsx`:
- Line 58: In the term-tooltip component's className string (the JSX attribute
on the element rendering the tooltip in term-tooltip.tsx), remove the accidental
double space between "bg-zinc-800/70" and "rounded-md" so the className tokens
are separated by a single space; update the className value on that JSX element
accordingly to avoid the extra whitespace.
---
Nitpick comments:
In `@frontend/app/view/term/term-tooltip.tsx`:
- Around line 90-95: The helper clearMaxTimeout is being recreated every render
even though it only uses the stable ref maxTimeoutRef; make it stable by
memoizing it (e.g. wrap clearMaxTimeout in useCallback with an empty dependency
array) or move its logic inline into the effect that needs it, ensuring
references remain to maxTimeoutRef.current and you still call
window.clearTimeout and set maxTimeoutRef.current = null; update any callers to
use the new stable clearMaxTimeout reference.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
frontend/app/element/errorboundary.tsxfrontend/app/view/term/term-model.tsfrontend/app/view/term/term-tooltip.tsxfrontend/app/view/term/term.tsxfrontend/app/view/term/termwrap.ts
| if (hoveredURL) { | ||
| menu.push({ | ||
| label: "Open URL (" + selectionURL.hostname + ")", | ||
| label: "Open URL (" + hoveredURL.hostname + ")", | ||
| click: () => { |
There was a problem hiding this comment.
hoveredURL.hostname is empty for non-HTTP(S) URLs, producing "Open URL ()".
file://, ssh://, or other custom-scheme URIs have an empty hostname, resulting in a misleading label. Guard with a fallback.
🛠 Proposed fix
- label: "Open URL (" + hoveredURL.hostname + ")",
+ label: "Open URL (" + (hoveredURL.hostname || hoveredURL.href) + ")",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (hoveredURL) { | |
| menu.push({ | |
| label: "Open URL (" + selectionURL.hostname + ")", | |
| label: "Open URL (" + hoveredURL.hostname + ")", | |
| click: () => { | |
| if (hoveredURL) { | |
| menu.push({ | |
| label: "Open URL (" + (hoveredURL.hostname || hoveredURL.href) + ")", | |
| click: () => { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/app/view/term/term-model.ts` around lines 796 - 799, The menu label
building uses hoveredURL.hostname which is empty for non-HTTP(S) schemes; update
the label in the menu.push call (the block that constructs "Open URL (" +
hoveredURL.hostname + ")") to fall back to a meaningful value such as
hoveredURL.href (or hoveredURL.protocol + hoveredURL.pathname) when
hoveredURL.hostname is falsy so the label becomes "Open URL (…)" with the actual
URI for file://, ssh://, and custom schemes.
| menu.push({ type: "separator" }); | ||
| const hoveredLinkUri = this.termRef.current?.hoveredLinkUri; | ||
| if (hoveredLinkUri) { | ||
| let hoveredURL: URL = null; |
There was a problem hiding this comment.
CRITICAL: TypeScript type error - cannot assign null to type URL
The variable is declared as let hoveredURL: URL = null; but URL type cannot be null. This should be URL | null to allow null assignment.
| let hoveredURL: URL = null; | |
| let hoveredURL: URL | null = null; |
There was a problem hiding this comment.
We are not using strict mode for typescript, so null assignment is fine.
|
|
||
| // ── wired-up sub-component ─────────────────────────────────────────────────── | ||
|
|
||
| function clearTimeoutRef(ref: React.RefObject<number | null>) { |
There was a problem hiding this comment.
WARNING: Incorrect type signature for clearTimeoutRef
The function parameter is typed as React.RefObject<number | null> but the function assigns to ref.current at line 73, which requires React.MutableRefObject (not RefObject, which has a readonly current property). The callers at lines 104, 107, 117, and 127 pass React.MutableRefObject from useRef.
| function clearTimeoutRef(ref: React.RefObject<number | null>) { | |
| function clearTimeoutRef(ref: React.MutableRefObject<number | null>) { |
There was a problem hiding this comment.
MutableRefObject is no longer a thing in React 19.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
frontend/app/view/term/term-tooltip.tsx (1)
36-47: Addrefsto theuseLayoutEffectdependency array.Line [47] omits
refswhile the effect callsrefs.setPositionReference(...), which can trip exhaustive-deps and risks stale captures.🔧 Proposed fix
- }, [isOpen, mousePos?.x, mousePos?.y]); + }, [isOpen, mousePos?.x, mousePos?.y, refs]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/view/term/term-tooltip.tsx` around lines 36 - 47, The effect in useLayoutEffect references refs.setPositionReference but refs is missing from the dependency array; update the dependency array for the useLayoutEffect (the hook that creates virtualEl) to include refs (or refs.setPositionReference) so the effect reruns when the refs object changes; alternatively destructure const { setPositionReference } = refs above and include setPositionReference in the dependency array to avoid stale captures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@frontend/app/view/term/term-tooltip.tsx`:
- Around line 36-47: The effect in useLayoutEffect references
refs.setPositionReference but refs is missing from the dependency array; update
the dependency array for the useLayoutEffect (the hook that creates virtualEl)
to include refs (or refs.setPositionReference) so the effect reruns when the
refs object changes; alternatively destructure const { setPositionReference } =
refs above and include setPositionReference in the dependency array to avoid
stale captures.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/app/element/errorboundary.tsxfrontend/app/view/term/term-model.tsfrontend/app/view/term/term-tooltip.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/app/element/errorboundary.tsx
1 participant
addresses issue #2901